Investigate flaky download test by asolntsev · Pull Request #16716 · SeleniumHQ/selenium

asolntsev · 2025-12-10T19:48:59Z

User description

💥 What does this PR do?

Investigate this flaky test:

canDownloadFiles() (org.openqa.selenium.grid.router.RemoteWebDriverDownloadTest)
org.opentest4j.AssertionFailedError: 
Expecting actual:
  ""
to be equal to:
  "Hello, World!"
when ignoring newlines (\n, \r\n).
	at org.openqa.selenium.grid.router.RemoteWebDriverDownloadTest.canDownloadFiles(RemoteWebDriverDownloadTest.java:163)

e.g. https://github.com/SeleniumHQ/selenium/actions/runs/20090736672/job/57637952727?pr=16713

🔄 Types of changes

Bug fix (backwards compatible)

PR Type

Bug fix, Tests

Description

Add diagnostic logging to investigate flaky download test
Convert canDownloadFiles test to repeated test (20 iterations)
Improve WebDriverWait polling with explicit poll interval
Temporarily disable non-Java CI workflows for investigation

File Walkthrough

Relevant files

Tests

RemoteWebDriverDownloadTest.java `Add diagnostics and repeat flaky download test` java/test/org/openqa/selenium/grid/router/RemoteWebDriverDownloadTest.java Added diagnostic logging to print found files and matching files during download wait Changed `canDownloadFiles()` from single `@Test` to `@RepeatedTest(20)` for flakiness investigation Enhanced `WebDriverWait` with explicit poll interval of 50ms for more responsive polling Refactored stream collection to use static import `toList()` and improved code readability Consolidated import statements for JUnit Jupiter annotations	+17/-15

Configuration changes

ci-java.yml `Disable macOS and remote CI jobs temporarily` .github/workflows/ci-java.yml Removed macOS browser tests job entirely Removed remote tests job entirely Removed FederatedCredentialManagementTest from Windows test run Kept only Windows CI job with RemoteWebDriverDownloadTest focus	+0/-44
ci.yml `Disable non-Java CI workflows temporarily` .github/workflows/ci.yml Removed .NET CI workflow job Removed Python CI workflow job Removed Ruby CI workflow job Removed Rust CI workflow job Kept only Java CI workflow active	+0/-50

qodo-code-review · 2025-12-10T19:49:22Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Diagnostic logging: Added diagnostic logging prints to stdout which are not structured audit logs and may not align with audit trail requirements though this is test code. Referred Code HasDownloads hasDownloads = (HasDownloads) driver; new WebDriverWait(driver, Duration.ofSeconds(5), Duration.ofMillis(50)) .until( d -> { List<String> files = hasDownloads.getDownloadableFiles(); List<String> matchingFiles = files.stream() .filter((f) -> FILE_EXTENSIONS.stream().anyMatch(f::endsWith)) .collect(toList()); System.out.printf( "[*****] FOUND %s FILES: %s; MATCHING %s FILES: %s%n", files.size(), files, matchingFiles.size(), matchingFiles); // ensure we hit no temporary file created by the browser while downloading return matchingFiles.size() == 2; }); Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Test wait logic: The polling wait assumes exactly two matching files without explicit handling/logging for timeouts beyond a console print, which may hinder diagnosing certain edge cases. Referred Code HasDownloads hasDownloads = (HasDownloads) driver; new WebDriverWait(driver, Duration.ofSeconds(5), Duration.ofMillis(50)) .until( d -> { List<String> files = hasDownloads.getDownloadableFiles(); List<String> matchingFiles = files.stream() .filter((f) -> FILE_EXTENSIONS.stream().anyMatch(f::endsWith)) .collect(toList()); System.out.printf( "[*****] FOUND %s FILES: %s; MATCHING %s FILES: %s%n", files.size(), files, matchingFiles.size(), matchingFiles); // ensure we hit no temporary file created by the browser while downloading return matchingFiles.size() == 2; }); Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Unstructured logs: Uses System.out.printf to log lists of filenames which are unstructured and could expose file names; consider structured logging or redaction even in tests if sensitive. Referred Code System.out.printf( "[*****] FOUND %s FILES: %s; MATCHING %s FILES: %s%n", files.size(), files, matchingFiles.size(), matchingFiles); // ensure we hit no temporary file created by the browser while downloading Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

qodo-code-review · 2025-12-10T19:50:51Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Revert disruptive CI configuration changes The suggestion advises reverting the CI configuration changes in files like `ci.yml` and `ci-java.yml`. These changes disable tests for multiple languages and platforms, which is unsuitable for a final merge as it compromises regression testing. Examples: .github/workflows/ci.yml [48-58] java: name: Java needs: check uses: ./.github/workflows/ci-java.yml if: > github.event_name == 'schedule' \|\| github.event_name == 'workflow_dispatch' \|\| github.event_name == 'workflow_call' \|\| contains(needs.check.outputs.targets, '//java') \|\| contains(join(github.event.commits.*.message), '[java]') \|\| ... (clipped 1 lines) .github/workflows/ci-java.yml [24-31] run: \| fsutil 8dot3name set 0 bazel test --flaky_test_attempts 3 --pin_browsers=true //java/test/org/openqa/selenium/chrome:ChromeDriverFunctionalTest ` //java/test/org/openqa/selenium/firefox:FirefoxDriverBuilderTest ` //java/test/org/openqa/selenium/grid/router:RemoteWebDriverDownloadTest ` //java/test/org/openqa/selenium/remote:RemoteWebDriverBuilderTest ` //java/test/org/openqa/selenium/grid/router:RemoteWebDriverDownloadTest ` //java/test/org/openqa/selenium/devtools:NetworkInterceptorRestTest Solution Walkthrough: Before: # .github/workflows/ci.yml jobs: check: ... dotnet: name: .NET ... java: name: Java ... python: name: Python ... ruby: name: Ruby ... rust: name: Rust ... After: # .github/workflows/ci.yml jobs: check: ... java: name: Java ... Suggestion importance[1-10]: 10 __ Why: The suggestion correctly identifies that the PR disables most of the CI pipeline, which is a critical issue, and rightly advises reverting these temporary debugging changes before merging.	High
Learned best practice	Ensure driver cleanup with finally Wrap driver usage in try/finally and call driver.quit() in finally so the session closes even if waits or assertions fail. java/test/org/openqa/selenium/grid/router/RemoteWebDriverDownloadTest.java [105-135] WebDriver driver = new RemoteWebDriver(gridUrl, capabilities); driver = new Augmenter().augment(driver); +try { + driver.get(appServer.whereIs("downloads/download.html")); + driver.findElement(By.id("file-1")).click(); + driver.findElement(By.id("file-2")).click(); -driver.get(appServer.whereIs("downloads/download.html")); -driver.findElement(By.id("file-1")).click(); -driver.findElement(By.id("file-2")).click(); + HasDownloads hasDownloads = (HasDownloads) driver; + new WebDriverWait(driver, Duration.ofSeconds(5), Duration.ofMillis(50)) + .until( + d -> { + List<String> files = hasDownloads.getDownloadableFiles(); + List<String> matchingFiles = + files.stream() + .filter((f) -> FILE_EXTENSIONS.stream().anyMatch(f::endsWith)) + .collect(toList()); + System.out.printf( + "[***] FOUND %s FILES: %s; MATCHING %s FILES: %s%n", + files.size(), files, matchingFiles.size(), matchingFiles); + return matchingFiles.size() == 2; + }); -HasDownloads hasDownloads = (HasDownloads) driver; -new WebDriverWait(driver, Duration.ofSeconds(5), Duration.ofMillis(50)) - .until( - d -> { - List<String> files = hasDownloads.getDownloadableFiles(); - List<String> matchingFiles = - files.stream() - .filter((f) -> FILE_EXTENSIONS.stream().anyMatch(f::endsWith)) - .collect(toList()); - System.out.printf( - "[***] FOUND %s FILES: %s; MATCHING %s FILES: %s%n", - files.size(), files, matchingFiles.size(), matchingFiles); - // ensure we hit no temporary file created by the browser while downloading - return matchingFiles.size() == 2; - }); + List<String> downloadableFiles = hasDownloads.getDownloadableFiles(); + assertThat(downloadableFiles).contains("file_1.txt", "file_2.jpg"); -List<String> downloadableFiles = hasDownloads.getDownloadableFiles(); -assertThat(downloadableFiles).contains("file_1.txt", "file_2.jpg"); + List<DownloadedFile> downloadedFiles = hasDownloads.getDownloadedFiles(); + assertThat(downloadedFiles.stream().map(f -> f.getName()).collect(toList())) + .contains("file_1.txt", "file_2.jpg"); +} finally { + driver.quit(); +} -List<DownloadedFile> downloadedFiles = hasDownloads.getDownloadedFiles(); -assertThat(downloadedFiles.stream().map(f -> f.getName()).collect(toList())) - .contains("file_1.txt", "file_2.jpg"); - -driver.quit(); - `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Always ensure external resources (e.g., WebDriver sessions) are cleaned up via try/finally to prevent leaks when assertions or waits fail.	Low
Possible issue	Remove duplicated test from CI Remove the duplicate `//java/test/org/openqa/selenium/grid/router:RemoteWebDriverDownloadTest` target from the `bazel test` command to avoid running the same test suite twice and reduce CI job duration. .github/workflows/ci-java.yml [26-31] bazel test --flaky_test_attempts 3 --pin_browsers=true //java/test/org/openqa/selenium/chrome:ChromeDriverFunctionalTest ` //java/test/org/openqa/selenium/firefox:FirefoxDriverBuilderTest ` //java/test/org/openqa/selenium/grid/router:RemoteWebDriverDownloadTest ` //java/test/org/openqa/selenium/remote:RemoteWebDriverBuilderTest ` - //java/test/org/openqa/selenium/grid/router:RemoteWebDriverDownloadTest ` //java/test/org/openqa/selenium/devtools:NetworkInterceptorRestTest `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a redundant test target in the CI configuration, which unnecessarily increases job duration.	Low
Update

[temporarily] run only RemoteWebDriverDownloadTest to investigate it

8db1cb1

selenium-ci added C-java Java Bindings B-build Includes scripting, bazel and CI integrations labels Dec 10, 2025

qodo-code-review Bot added the Review effort 2/5 label Dec 10, 2025

log the found files during download

b0a8032

asolntsev force-pushed the investigate-flaky-download-test branch from e56cee3 to b0a8032 Compare December 10, 2025 20:02

asolntsev marked this pull request as draft December 11, 2025 17:06

asolntsev added 2 commits December 11, 2025 21:19

reformat the code

1b002bf

reformat the code

9291893

asolntsev closed this Dec 16, 2025

asolntsev deleted the investigate-flaky-download-test branch December 16, 2025 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Investigate flaky download test#16716

Investigate flaky download test#16716
asolntsev wants to merge 4 commits intoSeleniumHQ:trunkfrom
asolntsev:investigate-flaky-download-test

asolntsev commented Dec 10, 2025 •

edited by qodo-code-review Bot

Loading

Uh oh!

qodo-code-review Bot commented Dec 10, 2025 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Dec 10, 2025 •

edited

Loading

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

asolntsev commented Dec 10, 2025 • edited by qodo-code-review Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

💥 What does this PR do?

🔄 Types of changes

PR Type

Description

File Walkthrough

Uh oh!

qodo-code-review Bot commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Compliance Guide 🔍

Uh oh!

qodo-code-review Bot commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

asolntsev commented Dec 10, 2025 •

edited by qodo-code-review Bot

Loading

qodo-code-review Bot commented Dec 10, 2025 •

edited

Loading

qodo-code-review Bot commented Dec 10, 2025 •

edited

Loading